Skip to content

Honor jdk.xml.overrideDefaultParser in the hardened source rewrites - #59

Merged
garydgregory merged 14 commits into
mainfrom
feature/restore-override-default-parser
Aug 28, 2026
Merged

Honor jdk.xml.overrideDefaultParser in the hardened source rewrites#59
garydgregory merged 14 commits into
mainfrom
feature/restore-override-default-parser

Conversation

@ppkarwasz

Copy link
Copy Markdown
Member

Stacked on #57 (uses its newDefault(NS)Instance methods).

The hardening wrappers rewrite every reader-less Source through their own hardened parsers, so the JDK TrAX, XPath and schema implementations' internal parsers are never used — and jdk.xml.overrideDefaultParser was silently ignored: on the stock JDK the feature's false default means internal parses use the JDK built-in parser, while the rewrite unconditionally used the ServiceLoader lookup.

@garydgregory
garydgregory force-pushed the feature/restore-override-default-parser branch from dbee6b0 to 658507c Compare August 28, 2026 10:51

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"jdk.xml.overrideDefaultParser" is a Java 21 feature. Does PR means we implement it on Java < 21? Is that intended? At least documented?

Base automatically changed from feature/jaxp-factory-methods to main August 28, 2026 11:00
The wrappers parse every reader-less Source themselves, so the wrapped
implementations' internal parsers are never used and the feature was
silently ignored. Instead of setting it on the underlying implementation
(the XPath hardener no longer does), the TrAX, XPath and schema wrappers
now read it at product creation, like the JDK: where the implementation
recognizes the feature and its value is false (the JDK's default), the
rewrites pin the platform's built-in parser via newDefaultNSInstance;
otherwise they keep the pluggable newNSInstance lookup. The
javax.xml.parsers.*Factory system properties override the pin, matching
the JDK's own internal parser choice.

Adds the test-jdk-xerces surefire execution, the only cell pairing the
JDK TrAX and XPath implementations with a third-party ServiceLoader
parser, where the two rewrite parser families genuinely differ.

Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErRKq7RUeQ9LGrSboSUyYm
newDefaultInstance() pins the JDK's XSLTC, which defines the compiled
translet class at run time; a closed-world native image cannot, which is
the reason the native-xalan profile substitutes Xalan for TrAX. The
capture tests stay enabled: newTemplates never loads the translet.

Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErRKq7RUeQ9LGrSboSUyYm
@garydgregory
garydgregory force-pushed the feature/restore-override-default-parser branch from 658507c to cd0f123 Compare August 28, 2026 11:00
@ppkarwasz

Copy link
Copy Markdown
Member Author

No, this feature is already present in JDK 8 (see Third party parsers).

JDK JAXP implementations are “sticky” by default. A JDK TraX implementation will not use an external Xerces parser.

Keeps main's MethodHandleFactory consolidation of the newDefaultInstance
lookup alongside this branch's overrideDefaultParser feature constant.

Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErRKq7RUeQ9LGrSboSUyYm

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppkarwasz
Maybe I've not had enough ☕ but IMO this needs better docs internally, for users, and the PR description.

I can't understand what we do and don't do when the value is true and false.

Comment thread src/site/markdown/threat_model.md Outdated
Comment thread src/main/java/org/apache/commons/xml/HardeningXPathFactory.java Outdated
Comment thread src/main/java/org/apache/commons/xml/HardeningXPathFactory.java Outdated
A returned factory is not necessarily an instance of the underlying
implementation, but everything except the security behavior is
preserved, including each implementation's internal parser choice: the
stock JDK's jdk.xml.overrideDefaultParser feature and system property
are honored, and Saxon keeps its own parser selection.

Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErRKq7RUeQ9LGrSboSUyYm
Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErRKq7RUeQ9LGrSboSUyYm
Both parser factories now expose the selection as a package-private
newNSInstance(boolean) whose Javadoc explains the role of the
javax.xml.parsers.*Factory system properties: they are the JDK's own
mechanism for reconfiguring what "default parser" means, so they are
honored through the standard lookup rather than bypassed.

Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErRKq7RUeQ9LGrSboSUyYm
…ature

The flag now carries the jdk.xml.overrideDefaultParser value itself
instead of its negation, so wrappers, floors and tests read the same
polarity as the JDK.

Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErRKq7RUeQ9LGrSboSUyYm
Neither harden() reads the feature; the selection logic is documented on
the wrappers' overrideDefaultParser() methods.

Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErRKq7RUeQ9LGrSboSUyYm
@ppkarwasz
ppkarwasz requested a review from garydgregory August 28, 2026 15:14
Clarified description of factory instances and their behavior.
Clarify the explanation about factory instances and their behavior.
@garydgregory
garydgregory merged commit 3620dda into main Aug 28, 2026
18 checks passed
@garydgregory
garydgregory deleted the feature/restore-override-default-parser branch August 28, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants